Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #837 +/- ##
==========================================
- Coverage 88.50% 88.29% -0.21%
==========================================
Files 46 47 +1
Lines 6505 6554 +49
==========================================
+ Hits 5757 5787 +30
- Misses 505 518 +13
- Partials 243 249 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The implementation preserves wrapper semantics, handles platform fallbacks, and includes focused coverage for metadata propagation.
Pull request overview
Adds ECN metadata reception for ICE packets on supported Unix platforms while preserving wrapper behavior.
Changes:
- Reads IPv4/IPv6 ECN values from UDP ancillary data.
- Exposes packet metadata through
Conn.ReadWithAttributes. - Adds ECN parsing and end-to-end tests.
File summaries
| File | Description |
|---|---|
transport.go |
Adds the public ECN metadata API. |
candidate_base.go |
Propagates packet attributes through receive handling. |
ecn_unix.go |
Implements Unix UDP ancillary-data parsing. |
ecn_other.go |
Provides the unsupported-platform fallback. |
ecn_unix_test.go |
Tests ECN parsing and delivery. |
go.mod |
Promotes x/sys to a direct dependency. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Carry UDP ECN codepoints in packetio.Attributes and expose reusable Conn.ReadWithAttributes reads. Handle IPv4 and IPv6 control messages on Unix.
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
Hello, I finished this and integrated it with the new transport attributes API, I put ECN behind a "unix" flag (linux || darwin || freebsd), I'm not sure how to do it for windows and I don't have a real way to test it on windows right now (I can do that later).
This part is the tricky part for supporting ECN. If the underlying socket is a UDPConn,
ReadMsgUDP(buf, oob)is used which returns out of band data like ToS. It works fine for now, but I honestly haven't thought about the cases where theconnis not a pure socket, but a class that implementsnet.PacketConn(like a Proxy or a Wrapper), and in this case we don't want to bypass the Wrapper's methods. However, for now, I think this is fine.